Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add argcheck and clean up code #397

Merged
merged 4 commits into from
Aug 12, 2022
Merged

Conversation

apcraig
Copy link
Contributor

@apcraig apcraig commented Aug 10, 2022

PR checklist

@phil-blain
Copy link
Member

This looks good, two comments:

  • do we want to add -Wall to some machines files in order to not introduce more unused variables ? How "clean" are we at the moment ? (also, which compiler was used to find these?)
  • I'm not sure I understand how this argcheck setting will be used. It would be nice to see an explanation:)

Copy link
Contributor

@dabail10 dabail10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lot of work! Looks reasonable to me. I guess there were some variables related to the deprecated options that we forgot about.

@apcraig
Copy link
Contributor Author

apcraig commented Aug 11, 2022

I used -Wall on gnu and cleaned up what was easy. There are some variables that are passed as arguments in subroutines that are not used, but I chose not to clean those up. There were other warning messages that also weren't reasonable to cleanup, like a return statement after an abort and before and output variable was set. -Wall did not reveal too many things. I thought about adding it to one of the machines, but decided not to. I think this is something we can run occasionally, but I don't think it's something we need to have on by default. @phil-blain, if you want to add it to one of your machines, that's fine :).

@apcraig
Copy link
Contributor Author

apcraig commented Aug 11, 2022

The argcheck parameter planning documentation is here,

https://github.com/CICE-Consortium/Icepack/files/9004010/Icepack_Package-2.pptx on slide 14.

The idea is that optional arguments checking could be turned off, done at "first call", or done at every call. We may add other options later. I agree some of this needs to be added to the user guide. I think we can do that as we make progress adding new optional arguments and seeing how that works out.

@apcraig apcraig merged commit 3a039e5 into CICE-Consortium:main Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants